New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add deriveaddresses RPC util method #14667
Conversation
I know, this comes up every time—see for example #14476 (comment) —wasn't the idea to not add more pure-utility RPC calls? |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
I created an issue to discuss a tool that could replace pure utility RPC calls: #14671 |
4f6fa31
to
9a55d5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, and after the discussion in the meeting this morning I think its worth adding.
utACK 9a55d5f
src/rpc/misc.cpp
Outdated
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid descriptor")); | ||
} | ||
if (desc->IsRange()){ | ||
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Descriptor range not supported")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be easy enough to add support for picking a specific index from a ranged descriptor, is there a reason you chose to disallow it? Just add a second parameter which specifies the index.
EDIT: thinking about it, it would be just as easy to not use a ranged descriptor if we're only using a single key, don't worry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a weird anti-feature to me. I don't understand why you're adding extra complexity to the function just to prevent a range. If you only want one, you can still do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially did that to prevent extra complexity, but I've actually encountered a situation where allowing ranges would be very useful. Will change this to return an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sjors I assume that also includes allowing combo
right? Sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meshcollider yes, no reason anymore to exclude combo()
doc/release-notes-13152.md
Outdated
@@ -3,3 +3,4 @@ New RPC methods | |||
|
|||
- `getnodeaddresses` returns peer addresses known to this node. It may be used to connect to nodes over TCP without using the DNS seeds. | |||
- `listwalletdir` returns a list of wallets in the wallet directory which is configured with `-walletdir` parameter. | |||
- `deriveaddress` returns the address corresponding to an [output descriptor](/doc/descriptors.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to belong in this file (the file is specific per PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listwalletdir was already hijacking that file, so I joined, but I can add a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case we could add them to the main file in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the point of these extra files was mainly to prevent constant rebasing, which seemed mostly a result of different sections interfering, and less when you're just adding one line an existing entry. Though I'm not sure exactly what trips up git.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was removed in master. Also, if another patch added a line in the meantime, git(hub) would also report a merge conflict.
9a55d5f
to
58d337b
Compare
Rebased. Result is now an array of addresses, so combo() and ranges can be used. Added range arguments. |
58d337b
to
50c656a
Compare
50c656a
to
f0e881f
Compare
Getting a rather cryptic linter error: @MarcoFalke? Update: #14718 was merged right before I pushed this. I rebased again and now I'm able to reproduce the linter error. It happens on master too. |
c71cec5
to
684fdd1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good
24a58ab
to
7eccdde
Compare
Rebased so CI works again. Renamed to plural |
7eccdde
to
82d2a8f
Compare
utACK 82d2a8f One last nit, I would add a comment about the default behavior of 0 for ranged descriptors if start and end aren't provided. EDIT: Actually, I think the helptext needs to be switched to RPCHelpMan |
@meshcollider alternatively I could make the range argument mandatory for ranged descriptors. What do you mean with RPCHelpMan? |
82d2a8f
to
dfad416
Compare
Rebased due to #14726 and using RPCHelpMan now. Range argument is now mandatory for ranged descriptors (typing |
Indeed, we could pass a lambda into the RPCHelpMan to validate the value and use the passed in type to validate the type (and thus get rid of |
71eef5a
to
7f78582
Compare
Rebased due to somewhat inexplicable Travis failure. |
7f78582
to
5952838
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of non-blocking nits, tACK 5952838
|
||
bare_multisig_descriptor = "multi(1, tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/0, tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/1/1)" | ||
assert_raises_rpc_error(-5, "Descriptor does not have a corresponding address", self.nodes[0].deriveaddresses, bare_multisig_descriptor) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a wallet roundtrip test like:
wpkh_info = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress("", "bech32"))
# Trim the witness part of descriptor off, check pkh entry in wallet, match descriptor
pkh_desc = wpkh_info["desc"][1:]
pkh_addr = self.nodes[0].deriveaddresses(pkh_desc)
pkh_info = self.nodes[0].getaddressinfo(pkh_addr)
assert_equal(wpkh_info["hdkeypath"], pkh_info["hdkeypath"])
assert_equal(wpkh_info["pubkey"], pkh_info["pubkey"])
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost) Pull request description: Usage: ```sh bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)" [ "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q" ] // part of the BIP32 test vector ``` Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors. ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~ As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR. Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34
Summary: This is a backport of Core [[bitcoin/bitcoin#14667 | PR14667]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6214
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost) Pull request description: Usage: ```sh bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)" [ "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q" ] // part of the BIP32 test vector ``` Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors. ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~ As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR. Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost) Pull request description: Usage: ```sh bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)" [ "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q" ] // part of the BIP32 test vector ``` Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors. ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~ As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR. Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost) Pull request description: Usage: ```sh bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)" [ "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q" ] // part of the BIP32 test vector ``` Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors. ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~ As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR. Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34
5952838 [rpc] util: add deriveaddresses method (Sjors Provoost) Pull request description: Usage: ```sh bitcoin-cli deriveaddresses "wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/0)" [ "bc1qg6ucjz7kgdedam7v5yarecy54uqw82yym06z3q" ] // part of the BIP32 test vector ``` Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with `scantxoutset` as a poor mans wallet. Might be useful to test more complicated future descriptors. ~To keep it as simple as possible it only supports descriptors that result in a single address, so no `combo()` and ranges.~ As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR. Tree-SHA512: b8e53db11a8fd87638cc98766270cc3be9adc4b3e5085798a6a4e2e6ad252bf6d2189346bbb2da72d04d13f7f1e80b5cb88e8039653bea1f150602a876ef7f34
Usage:
Avoids the need for external (BIP32) libraries to derive an address. Can be used in conjunction with
scantxoutset
as a poor mans wallet. Might be useful to test more complicated future descriptors.To keep it as simple as possible it only supports descriptors that result in a single address, so nocombo()
and ranges.As discussed recently on IRC it might make sense to put this in a separate utility along with other descriptor and psbt utility functions which don't need a chain or wallet context. However I prefer to leave that to another PR.